-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize nth_value
, remove first_value
, last_value
structs and use idiomatic rust style
#452
Conversation
Codecov Report
@@ Coverage Diff @@
## master #452 +/- ##
==========================================
+ Coverage 75.30% 75.36% +0.06%
==========================================
Files 152 152
Lines 25275 25294 +19
==========================================
+ Hits 19033 19063 +30
+ Misses 6242 6231 -11
Continue to review full report at Codecov.
|
nth_value
, remove first_value
, last_value
structs and use idiomatic rust style
@alamb as promised :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newly added tests are great. Thanks @jimexist -- and I think the code looks great too. 👍
fn test_i32_result(expr: Arc<NthValue>, expected: i32) -> Result<()> { | ||
let arr: ArrayRef = Arc::new(Int32Array::from(vec![1, -2, 3, -4, 5, -6, 7, 8])); | ||
let schema = Schema::new(vec![Field::new("arr", DataType::Int32, false)]); | ||
let batch = RecordBatch::try_new(Arc::new(schema), vec![arr])?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW for tests like this you can also use RecordBatch::try_from_iter
to avoid having to construct the Schema
directly.
This way is great too, I just figured I would point it out for the future
Which issue does this PR close?
Closes #448 .
Rationale for this change
we can:
What changes are included in this PR?
Are there any user-facing changes?